Add support for value_count for exponential_histogram field #136163
Conversation
elasticsearchmachine
added
external-contributor
elasticsearchmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
| return List.of(new ExponentialHistogramMapperPlugin()); | ||
| } | ||
|
|
||
| protected static List<ExponentialHistogram> createRandomHistograms(int count) { |
There was a problem hiding this comment.
I think I've seen a variant of this several times. Maybe extract to a common helper method? That could be in the exponential histogram lib test module and we can import the test module in the aggregations module like this:
Feel free to do this in a separate PR, whatever works best for you.
There was a problem hiding this comment.
Good point, there have been many variations of this. I'll try to put this in a lib in a separate PR
...pper-exponential-histogram/src/yamlRestTest/resources/rest-api-spec/test/20_aggregations.yml
Show resolved
Hide resolved
...pper-exponential-histogram/src/yamlRestTest/resources/rest-api-spec/test/20_aggregations.yml
Show resolved
Hide resolved
| @@ -0,0 +1,43 @@ | |||
| setup: | |||
|
|
|||
| # - skip: | |||
There was a problem hiding this comment.
This should skip everything before 9.3.0? Also do these yaml tests run in mixed cluster qa?
There was a problem hiding this comment.
The yaml tests are currently started with this class:
https://github.com/elastic/elasticsearch/blob/main/x-pack/plugin/mapper-exponential-histogram/src/yamlRestTest/java/org/elasticsearch/xpack/exponentialhistogram/ExponentialHistogramYamlTestSuiteIT.java
It skips testing when running on snapshot builds, which now in hindsight seems like the wrong approach.
I already had a chat with @not-napoleon about this and have the following refactor on my todo list:
- Add a test feature in MapperFeature for exponential histograms, which is present/absent based on the feature flag
- Move the yaml tests into the shared x-pack yaml tests and make them require the feature above
This should ensure that the tests work properly with e.g. mixed cluster qa and also when we remove the feature flag eventually.
I'll do this refactor after my current pile of queryDSL-aggs branches was reduced, otherwise it introduces a lot of conflicts for now. We'll be forced to do it when removing the feature flag (otherwise tests will start to fail)m so we can't forget about it.
.../main/java/org/elasticsearch/xpack/exponentialhistogram/ExponentialHistogramFieldMapper.java
Outdated
Show resolved
Hide resolved
…aluecount # Conflicts: # x-pack/plugin/mapper-exponential-histogram/src/main/java/org/elasticsearch/xpack/exponentialhistogram/ExponentialHistogramFieldMapper.java
4 participants
Part of #135625.
Adds support of the
exponential_histogramfor thevalue_countqueryDSL query.The implementation only loads the corresponding doc value and avoids loading the entire histogram.
This code was again heavily inspired from the corresponding implementation for
aggregate_metric_double